Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x clippy #117595

Merged
merged 6 commits into from
Dec 16, 2023
Merged

x clippy #117595

merged 6 commits into from
Dec 16, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 4, 2023 •
edited
Loading

thanks to @asquared31415 @albertlarsan68 for all their help, most of this pr is their work

note that this also adds x clippy --stage 0 -Awarnings to x86_64-gnu-llvm-15 to make sure it stays working; that won't gate on any clippy warnings, just enforce that clippy doesn't give a hard error. we can't add --stage 1 until clippy fixes its debug assertions not to panic.

note that x clippy --stage 1 currently breaks when combined with download-rustc.

unlike the previous prs, this doesn't require changes to clippy (it works by using RUSTC_WRAPPER instead), and supports stage 0

read this commit-by-commit

closes #107628; see also #106394, #97443. fixes #95988. helps with #76495.

r? bootstrap

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Nov 4, 2023
jyn514
jyn514 commented Nov 4, 2023
View reviewed changes
src/bootstrap/src/bin/rustc.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@matthiaskrgr
Copy link
Member

./x.py --stage 1 clippy

Does this build clippy from the repo to then run that clippy on rustc again?
I'm trying to figure out if we still have a way to use an up to date clippy on rustc without having to rebuild everything after each change to rustc/std sources with this pr.
I'm using that regularly to dogfood clippy against rustc after clippy updates.

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

Does this build clippy from the repo to then run that clippy on rustc again?

yes. --stage 0 is the version that doesn't build from source.

@matthiaskrgr
Copy link
Member

does stage0 always use "beta" or "whatever default toolchain is"?

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

"beta" (the bootstrap toolchain, sometimes stable)

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

if you want to test with a custom clippy without building rustc from source you can set build.rustc = "/path/to/custom/clippy/toolchain"

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@matthiaskrgr
Copy link
Member

I did some cursed stuff over at rust-lang/rust-clippy#11235 where I built clippy from the clippy repo, copy that one into the rustup-installed toolchain and then run x.py clippy on rustc to see if the upstream-clippy would crash at some point when checking rustc (for the clippy ci)

I'm curious to see if this will still work wrt this change, when I set the build.rustc to the rustc toolchain that clippy is pinned to, but I think it might...? :D
Anyway, that's something for another day...

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023 •
edited
Loading

I'm curious to see if this will still work wrt this change, when I set the build.rustc to the rustc toolchain that clippy is pinned to, but I think it might...? :D

it should, yes, but i haven't tested

@matthiaskrgr
Copy link
Member

When runnning x.py check it looks like we run with --all-targets(build and tests) do you think it makes sense to actually inherit that for x.py clippy?
I have not looked at the output of running clippy on the in-source tests yet, but I suspect some of the findings may be silly especially around std and core

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023 •
edited
Loading

When runnning x.py check it looks like we run with --all-targets(build and tests) do you think it makes sense to actually inherit that for x.py clippy?

that is what x.py clippy does today yes

i don't want to be the one who makes that decision

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@matthiaskrgr
Copy link
Member

Ah right makes sense that we would also run into some of the issues listed here rust-lang/rust-clippy#11235 (comment) 🥲

@jyn514
Copy link
Member Author

jyn514 commented Nov 5, 2023

yeah i'm gonna remove the stage 1 test until rust-lang/rust-clippy#11230 is fixed
stage 0 has debug assertions disabled so it should be ok to keep

@jyn514 jyn514 force-pushed the x-clippy branch 2 times, most recently from 0b807e2 to 1012698 Compare November 5, 2023 01:09
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 9, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
jyn514
jyn514 commented Dec 9, 2023
View reviewed changes
src/bootstrap/src/core/builder.rs Outdated
Comment on lines 1159 to 1162
// Set PATH to include the sysroot bin dir so clippy can find cargo.
let path = t!(env::join_paths(
// The sysroot comes first in PATH to avoid using rustup's cargo.
std::iter::once(PathBuf::from(initial_sysroot_bin))
.chain(env::split_paths(&t!(env::var("PATH"))))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc rust-lang/rust-clippy#11943

@bors
Copy link
Contributor

bors commented Dec 10, 2023

☔ The latest upstream changes (presumably #116278) made this pull request unmergeable. Please resolve the merge conflicts.

@jyn514
Copy link
Member Author

jyn514 commented Dec 10, 2023

won't have a chance to fix the conflicts for at least a week - would appreciate a review of the new changes before i do that if it's not too much trouble

@albertlarsan68
Copy link
Member

LGTM. Can you clean up the commits?
r=me once done.

@albertlarsan68 albertlarsan68 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2023
@saethlin
Copy link
Member

Commits look pretty clean to me
@bors r=albertlarsan68

@bors
Copy link
Contributor

bors commented Dec 16, 2023

📌 Commit a078c3a has been approved by albertlarsan68

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2023
@bors
Copy link
Contributor

bors commented Dec 16, 2023

⌛ Testing commit a078c3a with merge 4451777...

@bors
Copy link
Contributor

bors commented Dec 16, 2023

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 4451777 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 16, 2023
@bors bors merged commit 4451777 into rust-lang:master Dec 16, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 16, 2023
@jyn514 jyn514 deleted the x-clippy branch December 16, 2023 22:59
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4451777): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.6% [0.9%, 3.0%] 4
Regressions ❌
(secondary)		 1.9% [1.0%, 2.4%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.6% [0.9%, 3.0%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)		 -0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.5% [-0.5%, -0.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.679s -> 672.517s (-0.02%)
Artifact size: 312.39 MiB -> 312.37 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiaskrgr matthiaskrgr matthiaskrgr left review comments

@the8472 the8472 the8472 left review comments

@albertlarsan68 albertlarsan68 albertlarsan68 approved these changes

Assignees

@albertlarsan68 albertlarsan68

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.

Fix x.py clippy to be less janky